fix: async cache storing exception fixed #548
Conversation
| AsyncCache(Duration duration) : _duration = duration; | ||
| /// If [cacheErrors] is `false` the cache will be invalidated if the [Future] | ||
| /// returned by the callback completes as an error. | ||
| AsyncCache(Duration duration, {bool cacheErrors = true}) |
There was a problem hiding this comment.
Any naming we can use that would make the flag default to false?
Just because it's usually a good idea to have false as the default value.
Maybe bool ephemeralErrors = false. (Ouch, would hate to have to write that).
flushErrors?
|
|
||
| /// Creates a cache that invalidates after an in-flight request is complete. | ||
| /// | ||
| /// An ephemeral cache guarantees that a callback function will only be | ||
| /// executed at most once concurrently. This is useful for requests for which | ||
| /// data is updated frequently but stale data is acceptable. | ||
| AsyncCache.ephemeral() : _duration = null; | ||
| AsyncCache.ephemeral(): _duration = null, _cacheErrors = true; |
There was a problem hiding this comment.
The _cacheErrors here feels like it should be false (since it doesn't cache errors, because it doesn't cache anything). It's invisible to the user, so it doesn't really matter, and I guess it just means "don't special-case errors".
It's OK, just a little confusing.
I guess there are really three modes:
- Ephemeral everything.
- Ephemeral errors, persistent values.
- Persistent everything.
covered by two constructors. Should we just have three constructors?
| var errorThrowingFuture = cache.fetch(asyncFunctionThatThrows); | ||
| await expectLater(errorThrowingFuture, throwsA(isException)); | ||
|
|
||
| var valueFuture = cache.fetch(() async => 'Success'); |
There was a problem hiding this comment.
Consider checking that time hasn't progressed by one hour. Just for good measure.
| @@ -18,6 +18,20 @@ void main() { | |||
| cache = AsyncCache(const Duration(hours: 1)); | |||
| }); | |||
|
|
|||
| test('should not fetch when callback throws exception', () async { | |||
There was a problem hiding this comment.
Not sure "fetch" is the verb I'd use here (aka: I don't know what the description means).
It's consistent with the other tests. I also can't read those.
That's not really your issue to fix, though. (Should we say "recompute" instead of "fetch". Why is the method called fetch to begin with, seems to assume a specific use-case.)
PR Health
Breaking changes
|
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| async | Non-Breaking | 2.13.0 | 2.13.1-wip | 2.14.0 Got "2.13.1-wip" expected >= "2.14.0" (non-breaking changes) |
This check can be disabled by tagging the PR with skip-breaking-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
Coverage ⚠️
| File | Coverage |
|---|---|
| pkgs/async/lib/src/async_cache.dart | 💔 87 % ⬇️ 6 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
API leaks ✔️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbols |
|---|
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
| Files |
|---|
| no missing headers |
All source files should start with a license header.
Earlier async cache was caching exceptions.
With new changes in
fetchmethod ofAsyncCacheclass will no longer store exception.By default
AsyncCachewill store exception, by setting_canCacheExceptionto false. It won't store exception.Please refer #366 for video demo of both issue and solution along with minimal reproducible code.